Skip to content

[Refactor] Moving methods from Win32DPIUtil to DPIUtil for autoscaling #2709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Refactor] Moving methods from Win32DPIUtil to DPIUtil for autoscaling #2709

wants to merge 1 commit into from

Conversation

@ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Oct 30, 2025 •
edited
Loading

Workbench needs few methods from DPIUtil that were only present for Win32. Keeping the functionality for Win32DPIUtil as is, just making them available for DPIUtil.

Also removing the test that test some numeric autoscale value with monitor-specific scaling which is now not allowed.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025 •
edited
Loading

Test Results

  118 files  ±0    118 suites  ±0   17m 41s ⏱️ + 1m 22s
4 647 tests  - 3  4 630 ✅  - 3  17 💤 ±0  0 ❌ ±0 
  327 runs   - 3    323 ✅  - 3   4 💤 ±0  0 ❌ ±0 

Results for commit eceab5c. ± Comparison against base commit c3318b8.

This pull request removes 3 tests. 
org.eclipse.swt.widgets.ControlWin32Tests ‑ testAutoScaleImageData(float, String, boolean)[6]
org.eclipse.swt.widgets.ControlWin32Tests ‑ testAutoScaleImageData(float, String, boolean)[7]
org.eclipse.swt.widgets.ControlWin32Tests ‑ testAutoScaleImageData(float, String, boolean)[8]

♻️ This comment has been updated with latest results.

akurtakov
akurtakov requested changes Oct 30, 2025
View reviewed changes
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as draft October 30, 2025 12:30
@ShahzaibIbrahim ShahzaibIbrahim linked an issue Oct 30, 2025 that may be closed by this pull request
Limit monitor-specific scaling to supported autoscale modes vi-eclipse/Eclipse-Platform#399
Open
@ShahzaibIbrahim ShahzaibIbrahim mentioned this pull request Oct 30, 2025
Draft
1 task
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review October 30, 2025 14:47
HeikoKlare
HeikoKlare reviewed Oct 30, 2025
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of doing such a change more incrementally. Obviously, there is code in Platform UI depending on (internal) methods of DPIUtil. Moving them to another class means that Platform UI code becomes incompatible once we merge the SWT PR and if we merge the Platform UI PR at the same time (without knowing if it works as the CI will fail) will make Platform UI builds fail until the next I-Build.

Thus, in such a case a multi-step transition should be made by refactoring and maybe adding methods while still keeping the old ones (as delegates), then adapt the consumers (such as Platform UI) and once that is done remove the delegates that became obsolete.

It also seems like the AutoScaling class and DPIUtil would be highly mutually dependent. This could be an indicator that splitting them up is not a good idea. In any case, AutoScaling seems like it disposes much new public API that we will tie ourselves to. I know that I proposed to make some its functionality API, but seeing it I am not so sure anymore whether we should really do it or better keep it internal (like with DPIUtil) and access the internal API from Platform UI for now. At least, whatever we make public should be as lean as possible and in my opinion we should definitely not make something like an autoscale mode publicly accessible.

@ShahzaibIbrahim ShahzaibIbrahim changed the title [Refactor] Move autoscaling methods in AutoScaling class [Refactor] Moving methods from Win32DPIUtil to DPIUtil for autoscaling Oct 31, 2025
@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-399 branch 2 times, most recently from 7df9a79 to 08dcd65 Compare October 31, 2025 13:01
@ShahzaibIbrahim
Copy link
Contributor Author

As suggested, I have kept the methods in DPIUtil class. This PR now only moves few of the method in DPIUtil as these will be required to me in the workbench for subsequent PR: eclipse-platform/eclipse.platform.ui#3475.

The idea is to limit monitor specific for both SWT and eclipse products with compatible autoscaling methods.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-399 branch 4 times, most recently from 50b09f6 to bed297b Compare October 31, 2025 13:46
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as draft October 31, 2025 13:51
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review October 31, 2025 14:55
@ShahzaibIbrahim
Moving methods from Win32DPIUtil to DPIUtil for autoscaling
eceab5c 
Workbench needs few methods from DPIUtil that were only present for
Win32. Keeping the functionality for Win32DPIUtil as is, just making
them available for DPIUtil.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HeikoKlare HeikoKlare HeikoKlare left review comments

@akurtakov akurtakov akurtakov requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Limit monitor-specific scaling to supported autoscale modes

3 participants

@ShahzaibIbrahim @akurtakov @HeikoKlare